fix(ios): observe fill once without destructive repair - #3334

Size Report

Startup median (7 runs, lower is better):

The fill change in cb24ef6 is close, but it has one gap: a non-empty field baseline can now pass as a completed fill. CI is green, with all 21 checks passing at this head.

The early return for completed values in RunnerTests+TextEntryConfirmation.swift:66 runs before the isOrderedSubsequence(residualAndRequest, of: observed) residual check, and it ignores the baseline. The baseline is sampled after clearTextInput, so a partial clear can leave old text in the field. Take baseline "12", request "5678", and a field that shows "12 567 8". The grouping spaces count as inserted formatting, so textValueCompletesRequest returns true and unconfirmedTextEntryEvidence returns evidence. On main, the residual check classed this as an echo and the clear-and-retype repair ran. Now fill reports ok with verification "unconfirmed" and skips repair, while the field holds "125678" instead of "5678". That contradicts the new help and docs text that old text left behind by a failed clear fails. The stale-frame partial clear that #2634 describes can reach this. The rule the code must satisfy is that a value counts as completed only when the request plus inserted formatting explains the whole value, and text already in the field after the clear must never sit inside the completed span. The smallest fix is to run the residual check before textValueCompletesRequest when the baseline is non-empty, or to offer the completion exemption only when the post-clear baseline is empty. Please add an unconfirmedTextEntryEvidence case with baseline "12", request "5678", and observed "12 567 8" that expects nil. Every new evidence case uses baseline "", so none covers this, and I expect that case to fail at this head.

Not blocking: several new comments run 8 to 14 lines and narrate design rationale and review history, and AGENTS.md asks to keep that out of implementation comments, so one sentence stating the invariant would do; take or leave this.

On the open threads: the four cubic-dev-ai P3 threads are all fixed at this head, so you can resolve them: fixture caret restore, shared test helper, carriesRequest clause removed, and commands.md wording. The gap in the third thread's analysis is a different mechanism and is the finding above.

I did not run the XCTest suite or the device test revert, and I judged the regression by reading the main-branch classifier. I did not reproduce the finding on a device; it follows from the code and depends on clearTextInput leaving old text in a normalizing field. The issue names the examples/test-app phone field, while the PR uses a runner host-app fixture, and I accepted that as equivalent evidence without a test-app run. Before merge, the completion check needs to respect the post-clear baseline, with the "12" / "5678" / "12 567 8" case added.

Fixed in 9ef444a. Your arithmetic was right and I reproduced it before touching anything: at cb24ef6 the new case unconfirmedTextEntryEvidence(requested: "5678", baseline: "12", observed: "12 567 8") returned evidence instead of nil — the grouping spaces are real insertions, so textValueCompletesRequest granted the exemption while the field held residual "12" inside the value. Pushed as a red test first (it fails at cb24ef6, verified by running it against that head), then the fix.

Rather than ordering two checks around each other, the baseline is now a parameter of the deciding predicate itself: textValueCompletesRequest(observed:request:baseline:) refuses completion when any character the request's embedding did not consume is explainable by the baseline — the same shape as the existing foreign-character guard, so "residual may never stand inside the completed span" is decided in one place and cannot be asked without the fact it needs. Your case now dies at the first extra '1' (baseline '12'), falls through to the residual clause, and gets its repair like it does on main. The end-position argument from the old comment is gone; the doc now states the invariant instead of trusting where a mask parks residual text.

Chose the character-level guard over the empty-baseline-only gate deliberately: post-clear baselines are legitimately non-empty for summary fields, and when the value is fully explained by request + foreign insertions (e.g. baseline "5", observed "5 678" for "5678") the fill did work and disclosing is honest — the guard only refuses when a baseline character actually stands in the value unaccounted for. This is never looser than main: every refusal falls back to main's echo reading.

Pinned in the classifier tables: the refusal ("12" + "5678" → "12 567 8" → not a completion), its closest positive ("12" + "5678" → "5 678 12"... actually the empty-baseline positive "00 062 91 77" plus the absorbed-residual positive baseline "12" → "(555) 123-4567" for "5551234567", where the residual is consumed by the embedding and the extras are pure punctuation), so the rule is decided by the extras' provenance, not baseline emptiness.

On the comments: trimmed — the echo predicate, the completion predicate, the flipped pin, and the evidence test now carry one sentence per constraint; the narrative lives in this PR body instead. The four P3 threads: confirmed each fixed at this head myself before resolving (caret restore present in AgentDeviceRunnerApp.m, shared helper defined+used, CarriesRequest grep 0, commands.md wording 1 hit) and replied in-thread with the counts.

Validation at 9ef444a: red test fails at cb24ef6 / passes here; 19-test device run (11 iOS-lane selection incl. the grouping-field device test + 8 classifier/policy) all green; check:xctest-selection 0 unreachable; packaged-runner-swift ok; format/lint/typecheck clean. check:affected --run recorded below on this head.

Correction on one claim in my last comment: the red case was proven red by a local run against a build of cb24ef6 (the failure output above is from that run), but it landed as commits alongside the fix rather than as a separate pushed red commit — 9ef444a carries the predicate change plus the evidence-level case, and 3b98c12 adds the classifier-table pair (refusal "12"/"5678"/"12 567 8" and the absorbed-residual positive baseline "12" / "(555) 123-4567" for "5551234567"). Both the refusal and the positive are pinned at the completion-table and evidence levels now, so the rule is keyed on extra provenance, not baseline emptiness, and that choice is decided by tests rather than by prose.

This PR is ready for review at 3b98c12. The fix in 3b98c12 resolves the residual finding from the earlier review, and all 21 checks pass at that commit. The delta only touches the Swift classifier and its unit tests, which the iOS runner lanes exercise. Not blocking, and you can take or leave these: the docs and help sentence "A normalizing field is typed once, never cleared and retyped" (commands.md:477, and cli-help-workflows.ts:186) overclaims for masks that keep their template after a clear, for example baseline "$0.00", request "1000", observed "$10.00", where the delta refuses completion and the fill still ends in TEXT_ENTRY_MISMATCH as on main, so either limit the sentence to fields whose cleared value shares no formatting characters with the result or pin that case as a known limit in the classifier table. Also, the comment "A one-character request has no between for an insertion to sit in." at RunnerTests+TextEntryConfirmation.swift:90 now describes no code, since the count check moved into the opening guard, so it can go. I did not run the XCTest suite or a device run. The fail-before and pass-after claims rest on the PR body and my trace of the predicate, the live evidence uses the runner host-app grouping fixture rather than the reported Flutter field, and I reasoned about the template-retaining mask case from the code without reproducing it on a device. No conflicts. All four cubic-dev-ai P3 threads are fixed at this commit: the caret restore in AgentDeviceRunnerApp.m is untouched by this delta, the shared device-test helper is still in place, the carriesRequest clause is gone and 'old123456' is an echo again, and commands.md:477 now reads "to one that raw equality cannot reach".

Both taken, fixed in bbc93e3.

On the overclaim: the template case is not fixable at the rule in the forward direction, and your triple proves it — baseline "$0.00", request "1000", observed "$10.00" is the SAME textual relation as a failed clear that left only "$" in the field, where the typed 1000 renders as "$10.00" through the same mask. Surviving template and surviving residual are one string; only the app's intent distinguishes them and the runner cannot read masks. So I took your second option and made it testable rather than prose: the exact triple is pinned as a known limit at both the completion-table level and the evidence level (baseline "$0.00" / observed "$10.00" for request "1000" yields nil → repair path, same as main), the predicate doc names the boundary, and the docs/help sentence now reads: typed-once holds, with the template-shown-while-empty ("$0.00") field named as the limit that keeps failing with a repair. The residual-pair positives stay where they are — those cases have no baseline character standing in the value unexplained, which is exactly the fact the template case cannot establish.

Orphaned comment cut; the doc at :74 carries the one-character fact alone.

Validation at bbc93e3: 8 classifier/policy tests + the grouping device test green on iPhone 18 Pro / iOS 27.0; pnpm check:affected --run all runnable checks passed (recorded in the PR body); format/lint/typecheck clean.

The new commit bbc93e3 is clean. It fixes what the earlier review at 3b98c12 left open: the comments, the two known-limit assertions and the help and docs text now match what fill does.

All 21 checks pass at bbc93e3, and there are no conflicts. I did not run the XCTest unit tests. I checked the new known-limit assertions by tracing the predicate by hand. They pin existing behavior on purpose, so they would also pass before this change, which is fine for a limit pin. No device run reproduced the template-retaining mask. The claim that this case repairs and then mismatches rests on the code trace.

Nothing else is needed from you, and the PR is ready for a human to merge.

Both new threads answered and resolved at 63b8740.

F1 (submit-key guarantee scope): cubic's ordering claim verified against the implementation before adopting it — replacementTextEntryResult's identity guard (RunnerTests+TextEntryConfirmation.swift:89-90) returns verified: false (→ TEXT_ENTRY_MISMATCH) for a read resolving to a different element BEFORE the submit-key exception at :96 is reachable, pinned by testReplacementEvidenceRequiresTheSameReadableTarget. The guarantee was scoped by fixing the rule everywhere it is stated: 'observed on the same target' added identically at cli-help-workflows.ts:186 and website/docs/docs/commands.md:478; grep confirms no third surface states a submit-key sentence (metadata.ts, interaction/index.ts, fill-evidence.ts checked).

F2 (derived table expectation): the slow-read table now carries an explicit expected-unconfirmed field per row; nothing derives an outcome from the row's own input. Four slow-read/identity confirmation tests green on iPhone 18 Pro / iOS 27.0.

pnpm check:affected --run on 63b8740: all runnable checks passed — including command-docs (12 tests) at the head where the docs change exists, plus layering, fallow, build, 2267 vitest tests, xctest-selection, packaged-runner-swift.

Process note, answered with facts: the two commits at the previous head (c48bb4a, 402fe16) are authored under this machine's git identity but were not made in this worktree — my checkout sat at bbc93e3 until this turn fast-forwarded it. The pushing checkout is not listed in git worktree list on this machine, so it's either a detached-HEAD copy or a container/VM sharing the credentials; whoever operates it should either adopt a visible worktree or stop sharing the identity so drift checks can see it. My canonical checkout going forward is the tracked worktree: /Users/michal/.t3/worktrees/agent-device/fix-fill-normalizing-fields-2634, branch fix/fill-normalizing-fields-2634, now clean at 63b8740.

This PR is ready. The code at 63b8740 looks right to me, and the earlier review at bbc93e3 (#3334 (comment)) still holds. The retry wording, the late-first-read window, the removed-input log on the replacement path and the test columns are all fixed at this head.

Not blocking: a field whose value reaches the exact request only 0.8s to 1.0s after typeText returns, after a partial earlier read, still fails with TEXT_INPUT_COMMIT_NOT_OBSERVED (https://github.com/callstack/agent-device/blob/63b8740/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextEntryConfirmation.swift#L133), and the help text hard-codes --delay-ms 80 while the runner hint reads TextEntryTiming.recoveryDelayMilliseconds (https://github.com/callstack/agent-device/blob/63b8740/src/commands/schema/cli-help-workflows.ts#L186); take or leave both.

Of the earlier inline threads from cubic-dev-ai, none still applies. These are fixed at this head, so you can resolve them: the retry wording at #3334 (comment), the late first read at #3334 (comment), the removed-input log at #3334 (comment), the "observed on the same target" wording at #3334 (comment), and the test columns at #3334 (comment). This one is resolved as intended, because a submit-key mismatch returns verified:nil by design (documented in the help and docs, pinned by tests): #3334 (comment).

All 21 checks pass at 63b8740, and there are no conflicts. I did not run the XCTest unit or device suites. I judged the regression by reading the old repair path, which always repaired replacement mismatches. The iOS 27 fail-before and pass-after result comes from the PR body, and I could not tell whether any CI lane runs the device-hosted tests or only compiles them. Nothing else from review is open, so merging is up to the maintainer.

Answering the open question first, because it bounds my own evidence claim:

CI runs the device-hosted tests; it does not merely compile them. Three lanes reach the AgentDeviceRunnerUITests bundle, and scripts/check-xctest-selection.ts asserts every declared method is reachable by at least one (currently 0 unreachable, enforced per PR):

• ios.yml:151 executes xcodebuild test-without-building on a booted iOS simulator with a curated 104-method -only-testing: list generated via --ios-pr-tests. This lane ran on this PR: run 37984237505 succeeded at 63b874059.
• xctest-nightly.yml (cron 04:30): same command minus the -only-testing: flags — all 392 reachable methods on the simulator.
• macos.yml host lane: the whole macOS-compiled unit-test bundle, every PR.

Precisely which tests carry this PR's claims: the device-hosted fail-after claims (testFillIntoDigitGroupingFieldReportsUnconfirmedEvidenceAndDoesNotRepair, testFillIntoTemplateCurrencyFieldReportsUnconfirmedWithoutRetyping, the digit-count fills) are inside the curated --ios-pr-tests list, so the green-on-iOS-simulator half runs on every PR that routes here. The platform-neutral slow-read/identity policy tests are not in the iOS PR list by routing design (they are host-runnable) — they run on the macOS host lane every PR and on iOS nightly. The fail-before half is local-only: it was produced by reverting the fix and running the device test on an iPhone 18 Pro / iOS 27.0 simulator; no CI lane ever ran the reverted tree, so treat that half as author-run evidence, pinned in the PR body, not CI-covered.

Non-blocking items, dispositioned:

• (i) 0.8–1.0s window: left as is, deliberately. The shape is deadline = startedAt + replacementSettleCeiling (1.0) with the late-first-read extension only firing when sampledAt + verificationStabilityWindow (0.2) exceeds it (RunnerTests+TextEntryConfirmation.swift:57-60 — the cited :133 is inside textEntryRequestWithoutSubmitKeys), so a value arriving at 0.8–1.0 cannot accumulate a stability window before the deadline and takes the bounded TEXT_INPUT_COMMIT_NOT_OBSERVED fallback with the retry hint. That is the observation budget's designed edge: bounded runner commands over unbounded settling, fail-closed and disclosed. Widening it is a behavior change this PR should not smuggle.
• (ii) hard-coded --delay-ms 80 in cli-help-workflows.ts: confirmed real, and it is my line. Not fixed here: TS cannot import the Swift constant, the honest mechanism is the cross-language golden-table pattern AGENTS.md prescribes, and building that is a cross-module change this bugfix PR should not absorb. Filed as Pin the recovery --delay-ms constant across the Swift hint and the TS help text #3360 with the completion condition (changing recoveryDelayMilliseconds without the help text must fail a gate).

The five inline threads listed as resolvable were already resolved with in-thread verification replies — 10 threads, 0 unresolved — so nothing was spent there. Head and gates unchanged: 63b874059, check:affected --run recorded in the body.